-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ui): add support for custom field types #5113
Conversation
Node authors may now create their own arbitrary/custom field types. Any pydantic model is supported. Two notes: 1. Your field type's class name must be unique. Suggest prefixing fields with something related to the node pack as a kind of namespace. 2. Custom field types function as connection-only fields. For example, if your custom field has string attributes, you will not get a text input for that attribute when you give a node a field with your custom type. This is the same behaviour as other complex fields that don't have custom UIs in the workflow editor - like, say, a string collection.
We need to hold onto the original type of the field so they don't all just show up as "Unknown".
We have some fields that are have no UI - for example, some of the model fields are only used via connection and will never be used any other way. We could probably handle those fields as "custom" fields, per this PR. It wouldn't reduce any complexity exactly, but it would reduce verbosity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this and everything works as expected!
I'm glad to see that no backend changes were needed to support this.
There are couple of natural future improvements that I think would make this broadly more useful. You're probably already thinking about these, but figured I'd call them out anyways:
- UI should only allow connections between unknown types with the same type name. Right now you can connect incompatible unknown types and it won't be caught until graph execution time.
- I think we could hash the unknown type name and map it to a color to still get a unique color for each custom type in the UI.
...keai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/FieldHandle.tsx
Outdated
Show resolved
Hide resolved
Oops, #1 is unintentional. I'll figure out a fix for that in this PR as it may change the implementation. I suspect different colors for every type will end up looking rather chaotic, but it's worth trying out. I'll save that for the future. |
In the initial commit, a custom field's original type was added to the *field templates* only as `originalType`. Custom fields' `type` property was `"Custom"`*. This allowed for type safety throughout the UI logic. *Actually, it was `"Unknown"`, but I changed it to custom for clarity. Connection validation logic, however, uses the *field instance* of the node/field. Like the templates, *field instances* with custom types have their `type` set to `"Custom"`, but they didn't have an `originalType` property. As a result, all custom fields could be connected to all other custom fields. To resolve this, we need to add `originalType` to the *field instances*, then switch the validation logic to use this instead of `type`. This ended up needing a bit of fanagling: - If we make `originalType` a required property on field instances, existing workflows will break during connection validation, because they won't have this property. We'd need a new layer of logic to migrate the workflows, adding the new `originalType` property. While this layer is probably needed anyways, typing `originalType` as optional is much simpler. Workflow migration logic can come layer. (Technically, we could remove all references to field types from the workflow files, and let the templates hold all this information. This feels like a significant change and I'm reluctant to do it now.) - Because `originalType` is optional, anywhere we care about the type of a field, we need to use it over `type`. So there are a number of `field.originalType ?? field.type` expressions. This is a bit of a gotcha, we'll need to remember this in the future. - We use `Array.prototype.includes()` often in the workflow editor, e.g. `COLLECTION_TYPES.includes(type)`. In these cases, the const array is of type `FieldType[]`, and `type` is is `FieldType`. Because we now support custom types, the arg `type` is now widened from `FieldType` to `string`. This causes a TS error. This behaviour is somewhat controversial (see microsoft/TypeScript#14520). These expressions are now rewritten as `COLLECTION_TYPES.some((t) => t === type)` to satisfy TS. It's logically equivalent.
Fixing the validation was a bit more involved than I would have hoped, but not terrible. I updated the nodes in https://github.com/psychedelicious/test-arbitrary-fields with an extra field type to facilitate testing multiple custom field types. @RyanJDick Would you mind giving this a spin again? Notes: feat(ui): custom field types connection validation In the initial commit, a custom field's original type was added to the field templates only as *Actually, it was Connection validation logic, however, uses the field instance of the node/field. Like the templates, field instances with custom types have their To resolve this, we need to add This ended up needing a bit of fanagling:
While this layer is probably needed anyways, typing (Technically, we could remove all references to field types from the workflow files, and let the templates hold all this information. This feels like a significant change and I'm reluctant to do it now.)
Because we now support custom types, the arg This causes a TS error. This behaviour is somewhat controversial (see microsoft/TypeScript#14520). These expressions are now rewritten as |
Haven't tested thoroughly or looked at the code yet, but in my initial tests it wasn't working for list types. I.e. the following wasn't working: class CustomFieldOutput(BaseInvocationOutput):
"""Base class for nodes that output a single some field"""
my_field: list[CustomField] = OutputField() Edit: I don't think I ever tested this on the original commit, so this may have been an issues from the start. |
- Update connection validation for custom types - Use simple string parsing to determine if a field is a collection or polymorphic type. - No longer need to keep a list of collection and polymorphic types. - Added runtime checks in `baseinvocation.py` to ensure no fields are named in such a way that it could mess up the new parsing
…onStartFieldType' This was confusingly named and kept tripping me up. Renamed to be consistent with the `reactflow` `ConnectionStartParams` type.
Yep, I forgot about Collection and Polymorphic types. I've updated the PR to handle these cases, and my custom node test repo to include both Collections and Polymorphics. (This part of the UI really could use some unit tests) |
"Custom", "CustomCollection" and "CustomPolymorphic" are reserved field names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested manually and didn't run into any problems.
I did my best to review, but have admittedly limited familiarity with how the frontend type handling works.
@@ -12,7 +12,7 @@ import { getIsGraphAcyclic } from './getIsGraphAcyclic'; | |||
const isValidConnection = ( | |||
edges: Edge[], | |||
handleCurrentType: HandleType, | |||
handleCurrentFieldType: FieldType, | |||
handleCurrentFieldType: FieldType | string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Here and in several other places in this PR we use FieldType | string
, which is equivalent to just string
.
I'd tend to just use the string
type. Unless you think including FieldType
improves readability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, just for readability. TS also widens it to string, so when you use intellisense, you also see string.
I could be convinced it's less readable than just string, though.
* If these inconsistencies were resolved, we could remove these mappings and use simple string | ||
* parsing/manipulation to handle field types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely convinced that this is a desired outcome. I definitely have less context than you do, but on the surface I'd tend towards not wanting to rely on any particular type name structure.
Happy to save this discussion for when the time comes though!
export const getIsPolymorphic = (type: FieldType | string): boolean => | ||
type.endsWith('Polymorphic'); | ||
|
||
export const getIsCollection = (type: FieldType | string): boolean => | ||
type.endsWith('Collection'); | ||
|
||
export const getBaseType = (type: FieldType | string): FieldType | string => | ||
getIsPolymorphic(type) | ||
? type.replace(/Polymorphic$/, '') | ||
: getIsCollection(type) | ||
? type.replace(/Collection$/, '') | ||
: type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm certain that I'm not aware of all of the constraints here, but seeing this makes me think that we should definitely try to move toward a structured type representation rather than relying on type names for everything.
i.e. something like:
type FieldType = {
name: string;
isCollection: boolean;
isPolymorphic: boolean;
// Maybe?
isCustom: boolean;
}
Is this something you're already thinking about? Are there constraints that make this hard to do today?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was wishing I made it like that initially. The collection and polymorphic support wasn't in the initial design so it was tacked on like you see now.
The only constraint is that existing workflows have field types baked in to them - another thing that probably isn't optimal, in hindsight. Changing this would need some migration logic.
I'm going to take a step back and think about this a bit more. I'm working on the workflow library now. It may be the best time to make some improvements like this.
superseded by #5175 |
What type of PR is this? (check all applicable)
Have you discussed this change with the InvokeAI team?
Description
Node authors may now create their own arbitrary/custom field types. Any pydantic model is supported.
Two notes:
Suggest prefixing fields with something related to the node pack as a kind of namespace.
For example, if your custom field has string attributes, you will not get a text input for that attribute when you give a node a field with your custom type.
This is the same behaviour as other complex fields that don't have custom UIs in the workflow editor - like, say, a string collection.
QA Instructions, Screenshots, Recordings
Clone this repo into your
invokeai/nodes/
dir: https://github.com/psychedelicious/test-arbitrary-fieldsIt contains two nodes:
Custom Field Test 1
Custom Field Test 2
These two nodes use a custom field called
CustomField
.Try adding those nodes and connecting them. Should work like any node.